#864 development constant tail bug#868
Conversation
…eds to be reduced by the tail
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #868 +/- ##
==========================================
+ Coverage 87.04% 87.19% +0.15%
==========================================
Files 86 86
Lines 4986 5030 +44
Branches 646 655 +9
==========================================
+ Hits 4340 4386 +46
+ Misses 456 455 -1
+ Partials 190 189 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nto #864_DevelopmentConstant_Tail_Bug
| if len(cdf_patterns) < tri_dev_periods: | ||
| warnings.warn( | ||
| "Supplied patterns are shorter than the triangle development " | ||
| "periods. Missing ages will be filled with a factor of 1.0.", | ||
| UserWarning, | ||
| stacklevel=2, | ||
| ) | ||
| tail_cdf = 1 | ||
|
|
||
| elif len(cdf_patterns) == tri_dev_periods: | ||
| tail_cdf = 1 |
There was a problem hiding this comment.
This warning is new, I thought about not giving one and just do it quietly, but I think this is better since I did the same in throwing a warning when I put in additional assumptions for the users in the cl.Development() method.
There was a problem hiding this comment.
On second look I think I can discard the tail_cdf = 1. That's already set above.
| if pattern_dev_periods < tri_dev_periods: | ||
| include_last = False | ||
| elif pattern_dev_periods == tri_dev_periods: | ||
| include_last = True |
There was a problem hiding this comment.
This is to address problem 2 in the main thread. Usually, the last age to age factor is not included (it assumes that the oldest origin row is at ultimate, e.g. if it's age 120, the last age to ultimate would be 102-ult, NOT 120-ult. Rewriting this broke a bunch of other things, so I kept that here.
If we need to somehow include 120-ult, because they are the same length, we'll do that here.
| else: | ||
| include_last = tail_cdf != 1 | ||
|
|
||
| dev_slice = slice(None) if include_last else slice(None, -1) |
There was a problem hiding this comment.
This is to slice that development, depends on if we drop the last factor (e.g. 120-ult).
| dev_slice = slice(None) if include_last else slice(None, -1) | ||
|
|
||
| # this is the object to fill out the patterns, skeleton frame | ||
| obj = obj.iloc[..., :1, dev_slice] * 0 + 1 |
There was a problem hiding this comment.
Now, we are back to the old method, obj is just the blank age to age (NOT age to ult) to fill out.
| ldf = ( | ||
| pd.concat( | ||
| [pd.DataFrame(item[0], index=[0]) for item in prepared], | ||
| axis=0, | ||
| ) | ||
| .fillna(1)[obj.ddims] | ||
| .values | ||
| ) | ||
| tail_cdfs = xp.array([item[1] for item in prepared]) | ||
|
|
||
| if self.callable_axis == 0: | ||
| ldf = xp.array(ldf[:, None, None, :]) | ||
| tail_cdfs = tail_cdfs[:, None, None] | ||
| else: | ||
| ldf = xp.array(ldf[None, :, None, :]) | ||
| tail_cdfs = tail_cdfs[None, :, None] |
There was a problem hiding this comment.
This is similar to the old stuff, I didn't touch.
| def _callable_row(row): | ||
| raw_patterns = self.patterns(row) | ||
| cdf_row, row_tail_cdf = self._prepare_cdf_patterns( | ||
| raw_patterns, tri_dev_periods | ||
| ) | ||
| fit_row = raw_patterns if self.style == "ldf" else cdf_row | ||
| return dict(fit_row), row_tail_cdf |
There was a problem hiding this comment.
The rows stuff is from the above, when it's callable. This is super annoying is because the pattern needs to stay the same in either LDF or CDF form, but just get the tail out.
| if not any(ddim == k or int(ddim) == int(k) for k in cdf_patterns): | ||
| cdf_patterns[int(ddim)] = 1.0 | ||
| if self.style == "ldf" and not any( | ||
| ddim == k or int(ddim) == int(k) for k in patterns | ||
| ): | ||
| patterns[int(ddim)] = 1.0 |
There was a problem hiding this comment.
Fill in the cdf patterns with 1.0 if something is missing (i.e. blank spaces).
| ): | ||
| patterns[int(ddim)] = 1.0 | ||
|
|
||
| fit_patterns = patterns if self.style == "ldf" else cdf_patterns |
There was a problem hiding this comment.
Still keeping track of both LDFs and CDFs.
| ldf = xp.array([float(fit_patterns[int(item)]) for item in obj.ddims]) | ||
| ldf = ldf[None, None, None, :] |
There was a problem hiding this comment.
Same as before.
| ldf = xp.concatenate((ldf[..., :-1] / ldf[..., 1:], ldf[..., -1:]), -1) | ||
|
|
||
| # apply tail_cdf to the last ldfs of the triangle | ||
| ldf[..., -1] = ldf[..., -1] * tail_cdfs |
There was a problem hiding this comment.
This is after following through everything, and just need to multiply the last element with the tail.
|
Hopefully this helps a bit. I can try to improve the readability to reduce tech debt a bit more tomorrow. |
|
@henrydingliu do you have Claude code? |
…nto #864_DevelopmentConstant_Tail_Bug
it's a lot of catching of corner cases. i actually just had an idea after reviewing your other PR. you know how @genedan created a fake 2nd latest diagonal in all the friedland data? what if we just take that idea to implement
i develop in databricks. so i just use the built-in AI for basic syntax type acceleration/debugging. this is what the databricks AI tells me when I ask what model it is. I’m an OpenAI-provided assistant accessed through Databricks, but the specific model identifier may be abstracted away by the platform. If you need the exact deployment/model name, the best place to check is your Databricks or API configuration where this assistant was set up. I also have a paid copilot 365. i do a lot of ideation and high-level solution design through that. Claude Opus is one of the models available. |
Hmm ok, I tried a few times to ask Cursor to refactor my code and it worked pretty terribly. I am going to go through another pass to do more cleanup but feel free to give it a try! |
| ).fit_transform(raa) | ||
| assert np.all( | ||
| np.round(result.cdf_.to_frame().values.flatten(), 6) | ||
| == np.array([1.1, 1.1, 1.1, 1.1, 1.1, 1.1, 1.1, 1.1, 1.1, 1.1]) |
There was a problem hiding this comment.
isn't this the same problem as what you originally wanted to address? i.e. fed 11 cdf into developmentconstant but only get 10 back
There was a problem hiding this comment.
ah, okay. the original code only gave you 9 back
There was a problem hiding this comment.
on third thought, i think this test actually shows a bug. the ldf_ would show 120-132 to be 1.1, which is not what was originally supplied.
There was a problem hiding this comment.
Sorry, you think the RHS is wrong? Why? The cdf from 120-ult is 1.1, as supplied?
There was a problem hiding this comment.
i'm not saying that the rhs is wrong. i'm saying this test isn't capturing an error in the untested ldf_. the ldf_ at 120 coming out of DevelopmentConstant is 1.1, but the actual ldf at 120 that was supplied to the estimator was 1.
| obj = self._set_fit_groups(X).val_to_dev().copy() | ||
|
|
||
| xp = obj.get_array_module() | ||
| obj = obj.iloc[..., :1, :-1]*0+1 |
There was a problem hiding this comment.
i think this is the only line you have to change.
if self.style == "cdf":
obj = obj.iloc[..., :1, :]*0+1
else:
obj = obj.iloc[..., :1, :-1]*0+1There was a problem hiding this comment.
Nah, that's the lazy way. You aren't going to catch all the edge cases. Just bring my tests in and try your code. You'll fail a bunch.
There was a problem hiding this comment.
obj = obj.iloc[..., :1, :]*0+1 is fundamentally changing the structure, this is now saying all development period will now develop one more, including the oldest origin period.
And if it's LDF style, then you don't? This is wrong.
There was a problem hiding this comment.
Try to bring my tests in, and you can see if you can catch all the edge cases more cleanly. I'm sure there's a way.
I think for this PR, reviewing the tests is actually more important than the code itself.
There was a problem hiding this comment.
Nah, that's the lazy way. You aren't going to catch all the edge cases. Just bring my tests in and try your code. You'll fail a bunch.
it definitely is the lazy way. it's also done in this PR, literally just a few lines down
obj = obj.iloc[..., :1, dev_slice] * 0 + 1
Try to bring my tests in, and you can see if you can catch all the edge cases more cleanly
good idea. i can do that
There was a problem hiding this comment.
Oh and if you want to try to use the tests and let AI solve it, you can give that a try if you have access to a good AI agent.
Cursor couldn't do it and just kept iterating itself until I killed it.
There was a problem hiding this comment.
Ha, it's very possible I made mistakes on the tests. So let's make sure the tests are right.
Your implementation is much cleaner, let's just make sure it can catch all the edge cases. I think we are super close.
There was a problem hiding this comment.
i think each test needs to test both the ldf_ and the cdf_. we'd just have to manually calculate the cdf/ldf from the supplied vector.
| 1.4641, | ||
| 1.331, | ||
| 1.21, | ||
| ] |
There was a problem hiding this comment.
i think this test actually shows a bug. the ldf_ would show 120-132 to be 1.21, which is not what was originally supplied.
There was a problem hiding this comment.
The CDF, not LDF, is 1.21. Line 365 checks the CDF.
There was a problem hiding this comment.
yes, the cdf is 121. that is correct. but the underlying ldf_ at 120 is also 1.21, which is not what was supplied to the estimator
There was a problem hiding this comment.
Let me check... I will add tests to check both the cdf_ and ldf_
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6f28c21. Configure here.
| else: | ||
| raise ValueError("callable axis needs to be 0 or 1") | ||
|
|
||
| patterns = self.patterns(rows.iloc[0]) |
There was a problem hiding this comment.
Callable path determines shape from first row only
Low Severity
When patterns is callable, self.patterns(rows.iloc[0]) is called to determine include_last and dev_slice based on the first row's pattern length. Each subsequent row is then independently processed in _callable_row, which may produce a different row_tail_cdf. If different rows return patterns of different lengths, the obj skeleton shape (determined solely by the first row) may be inappropriate for other rows — for example, if the first row's pattern is short (include_last=False) but another row's is long, the obj.ddims will have one fewer period than that row needs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6f28c21. Configure here.
| ) | ||
| assert np.all( | ||
| np.round(result.ldf_.to_frame().values.flatten(), 6) | ||
| == np.array([1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.1]) |
There was a problem hiding this comment.
this doesn't match "reported_pattern"
There was a problem hiding this comment.
It's not supposed to be, the reported_pattern is in CDF form, but the LHS/RHS check here is in LDF form.
There was a problem hiding this comment.
Let me know if you agree lol.
I had to be super careful, made many mistakes before. Just double check to see if you are aligned.
There was a problem hiding this comment.
the 1.1 comes at the 10th in the returned ldf_, instead of the 11th element implied by the original pattern
There was a problem hiding this comment.
Because the data object doesn't have the 11th origin period?
There was a problem hiding this comment.
right. so we have an issue here because the ldf_ returns changes, beyond filling with additional 1.0's, depending on how large the triangle is. in theory, we would want DevelopmentConstant().fit(trI_9x9).ldf_[:-1] to be equal to DevelopmentConstant().fit(trI_8x8).ldf_ but that's not happening
There was a problem hiding this comment.
Let me reply you on the main thread.
| 1.1, | ||
| 1.1, | ||
| 1.1, | ||
| 1.1**2, |
There was a problem hiding this comment.
this doesn't match reported_patterns
There was a problem hiding this comment.
It does? The only thing that's different is the last LDF, those need to be grouped, or it will be incorrectly discarded.
There was a problem hiding this comment.
there's no 1.21 in the original ldf pattern.
There was a problem hiding this comment.
Yes because the pattern extends beyond what is needed by the data object. Are you suggesting that you just discard the last one?
There was a problem hiding this comment.
no, the PR is what's discarding the last ldf. i'm saying we need to keep the last ldf, that would make it consistent with Development
raa = cl.load_sample('raa')
dev = cl.Development().fit(raa)
raa_1987 = raa[(raa.valuation <= '1988-01-01')]
print(dev.transform(raa).ldf_)
print(dev.transform(raa_1987).ldf_) 12-24 24-36 36-48 48-60 60-72 72-84 84-96 96-108 108-120
(All) 2.999359 1.623523 1.270888 1.171675 1.113385 1.041935 1.033264 1.016936 1.009217
12-24 24-36 36-48 48-60 60-72 72-84 84-96 96-108 108-120
(All) 2.999359 1.623523 1.270888 1.171675 1.113385 1.041935 1.033264 1.016936 1.009217
So I think the above is only true if the pattern supplied is shorter than a length of 7. Try this example, and comment/uncomment the pattern length, everything is working as expected to me. When pattern is raa10x10 = cl.load_sample("raa")
raa9x9 = raa10x10[raa10x10.valuation<'1990']
reported_patterns = {
12: 1.1,
24: 1.1,
36: 1.1,
48: 1.1,
60: 1.1,
72: 1.1,
84: 1.1,
96: 1.1,
108: 1.1,
# 120: 1.1,
# 132: 1.1,
}
raa10x10_ldf_ = cl.DevelopmentConstant(
patterns=reported_patterns, style="ldf"
).fit_transform(raa10x10).ldf_
raa9x9_ldf_ = cl.DevelopmentConstant(
patterns=reported_patterns, style="ldf"
).fit_transform(raa9x9).ldf_
print(raa10x10_ldf_)
print(raa9x9_ldf_)In raa10x10 = cl.load_sample("raa")
raa9x9 = raa10x10[raa10x10.valuation<'1990']
reported_patterns = {
12: 1.1,
24: 1.1,
36: 1.1,
48: 1.1,
60: 1.1,
72: 1.1,
84: 1.1,
96: 1.1,
108: 1.1,
# 120: 1.1,
# 132: 1.1,
}
raa10x10_ldf_ = cl.DevelopmentConstant(
patterns=reported_patterns, style="cdf"
).fit_transform(raa10x10).ldf_
raa9x9_ldf_ = cl.DevelopmentConstant(
patterns=reported_patterns, style="cdf"
).fit_transform(raa9x9).ldf_
print(raa10x10_ldf_)
print(raa9x9_ldf_) |
|
Sorry, but I am not following your question/feedback, but can you give me an example of where you think my implementation doesn't give the correct answer, and what the correct answer should be? |
right. when the supplied ldf or cdf is longer, the resulting ldf_ is distorted. right now you are forcing those tests to pass by comparing the resulting ldf_ to something other than what was supplied. |
this implementation doesn't give the right answer when the supplied pattern is longer. the correct answer should replicate the supplied ldf or the implied ldf of the suppled cdf exactly (not counting factors of 1 to fill the space). basically, if elsewhere in the package we want stuff like raa = cl.load_sample('raa')
dev = cl.Development().fit(raa)
raa_1987 = raa[(raa.valuation <= '1988-01-01')]
print(dev.transform(raa).ldf_)
print(dev.transform(raa_1987).ldf_) |
I don't think I agree with this. You are saying to just discard the extra LDF pattern beyond the triangle object? Look at this example: raa10x10 = cl.load_sample("raa")
raa9x9 = raa10x10[raa10x10.valuation<'1990']
reported_patterns = {
12: 1.1,
24: 1.1,
36: 1.1,
48: 1.1,
60: 1.1,
72: 1.1,
84: 1.1,
96: 1.1,
108: 1.1, #9th LDF, or if this is a CDF, it would've been 1.21
120: 1.1, #10th's LDF, not CDF
# 132: 1.1,
}
raa10x10_ldf_ = cl.DevelopmentConstant(
patterns=reported_patterns, style="ldf"
).fit_transform(raa10x10).ldf_
raa9x9_ldf_ = cl.DevelopmentConstant(
patterns=reported_patterns, style="ldf"
).fit_transform(raa9x9).ldf_
print(raa10x10_ldf_)
print(raa9x9_ldf_)Returns: 12-24 24-36 36-48 48-60 60-72 72-84 84-96 96-108 108-120 120-132
(All) 1.1 1.1 1.1 1.1 1.1 1.1 1.1 1.1 1.1 1.1
12-24 24-36 36-48 48-60 60-72 72-84 84-96 96-108 108-120
(All) 1.1 1.1 1.1 1.1 1.1 1.1 1.1 1.1 1.21
# ^ this 9th's LDF should
# be 1.1 instead of 1.21You are saying, we should just discard the remaining
This is different, in this example, you are estimating patterns using a set of data, and estimating another pattern using a subset of that data, it should be clear that the pattern estimated using the subset lacks something (i.e. the tail). Let me try to se if I can convince you with another example: Do you agree that these two patterns are the same? DC_LDF = cl.DevelopmentConstant(
patterns={
12: 1.1,
24: 1.1,
36: 1.1,
48: 1.1,
60: 1.1,
72: 1.1,
84: 1.1,
96: 1.1,
108: 1.1,
120: 1.1,
}, style="ldf"
)
DC_CDF = cl.DevelopmentConstant(
patterns={
12: 1.1**10,
24: 1.1**9,
36: 1.1**8,
48: 1.1**7,
60: 1.1**6,
72: 1.1**5,
84: 1.1**4,
96: 1.1**3,
108: 1.1**2,
120: 1.1,
}, style="cdf"
)If so, then you should get the same CDF, not LDF, no matter what triangle object you fit on. Again, in my opinion, one of the current implementation flaw that I think is that if the pattern provided is shorter, and it is in LDF form, the extra pattern is basically discarded. My PR fixes that. |


Summary of Changes
Addressed two bugs in the DevelopmentConstant()
Related GitHub Issue(s)
Fixes #864
Additional Context for Reviewers
This PR fixes both bugs, even though only 1 is reported on #864.
There was also an old bug(?) in
test_constant_callable_axis1, not sure whypatterns.valueshadpatterns.values[:, :-1]dropped the last value. This is corrected.uv run pytest) and documentation changes (uv run jb build docs --builder=custom --custom-builder=doctest)Note
Medium Risk
Changes core actuarial development-factor logic used in reserving workflows; behavior shifts for long LDF patterns and tails, though covered by extensive new tests.
Overview
DevelopmentConstant.fitis reworked so external CDF/LDF patterns align with triangle length and tails are preserved instead of dropped when patterns extend past the triangle.A new
_prepare_cdf_patternsconverts LDF inputs to CDFs, rebases in-triangle CDFs when a tail exists, and returns atail_cdfapplied to the last LDF.fitnow warns and fills missing ages with 1.0 when patterns are shorter than the triangle, handles callable patterns per row with the same tail logic, and supports incremental triangles vianot X.is_cumulative.test_constant.pygains broad regression coverage (exact/short/long patterns, tail vs no tail, LDF vs CDF, incremental) and fixestest_constant_callable_axis1to compare full CDF values without incorrectly dropping the last period.Reviewed by Cursor Bugbot for commit 6f28c21. Bugbot is set up for automated code reviews on this repo. Configure here.